-
Notifications
You must be signed in to change notification settings - Fork 51
Add tool-bar icons #237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add tool-bar icons #237
Conversation
* dape.el (dape-repl-mode): Add tool-bar icons. (dape-update-tool-bar): New function. (dape-tool-bar-map): New variable.
svaante
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution to dape.
How is your fsf status? See Readme.
I approve of the tool-bar and the general aproch, but I am not convince on what approach to add the tool-bar is best.
| If valid connection, this connection will be of highest priority when | ||
| querying for connections with `dape--live-connection'.") | ||
|
|
||
| (defvar dape-tool-bar-map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All defvar should contain docs
| (select-window window))))) | ||
|
|
||
|
|
||
| ;;; Tool bar config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just call this sections Tool bar?
| comint-prompt-regexp (concat "^" (regexp-quote dape--repl-prompt)) | ||
| comint-process-echoes nil) | ||
| comint-process-echoes nil | ||
| tool-bar-map dape-tool-bar-map) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should to the same in dape-shell-mode, dape-disassemble-mode and `dape-info-parent-mode'.
| (when (eq tool-bar-map dape-tool-bar-map) | ||
| (kill-local-variable 'tool-bar-map)))) | ||
| (unless (eq tool-bar-map dape-tool-bar-map) | ||
| (setq-local tool-bar-map dape-tool-bar-map)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine to have only dape-*-mode buffers having a tool bar but as dape does not select repl it is quite awkward to first select repl then use the buttons.
The reason I am bringing this up is that this will only prime the buffers opened when dape starts with the tool bar but not new buffers created by dape (Also we should restore local value of tool-bar-map if we overwrite it).
I see two options:
- Use repl as the selected starting window and skip this hook
- Use
find-file-hookto add the tool-bar to each new buffer (and buffers created fromdape-mime-mode-alist)
If we go with option (2.) we should probobly use modes key to figure out if a buffer should have the tool-bar.
WDYT? If we feel fine with using repl as the initial selection I think it's preferable to skip the bookkeeping needed for (2.).
If you have any other alternative ideas please share
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find using find-file-hook (2.) somewhat complex, since it will add the tool-bar icons to any unrelated file, for example an elisp file from another project.
I think that adding the icons to only the repl buffer would be the best choice, but the problem is that cursor have to be in the repl buffer (since tool-bar icons are buffer-local)
Another idea would be to use the header-line, since it is rarely used, and unlike the tool-bar, the icons are kept even if you're not in the repl buffer
I've already done my paperwork |
|
Sorry for dropping the ball on this one @DevelopmentCool2449, I do appreciate the work. I do think that find-file is a suitable option, we can use modes from config to decide which buffers get the header. Are you interested in implementing that + the other suggestions or are you fine with me picking up where you left off? Sorry for the late response! |
|
Sorry for dropping the ball on this one @DevelopmentCool2449, I do appreciate the work. I do think that
find-file is a suitable option, we can use modes from config to decide which buffers get the header.
Are you interested in implementing that + the other suggestions or are you fine with me picking up where
you left off?
Yeah i'm interested, i will see if i can implement it (i have been busy).
…--
- E.G via GNU Emacs and Org.
|
Currently this only add the toolbar icons in the dape repl buffer and in the buffer where
dapeis called instead only the buffers that are relevant.I'm not sure how to achieve that and I don't know if there is a function in dape source that can allow me to do that.
(closes: #171)